-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Client.stream writableNeedDrain #442
Conversation
Missing tests. WIP |
1d3eed1
to
3cb9234
Compare
68a5e14
to
0e03afc
Compare
0e03afc
to
76010a2
Compare
d86c660
to
03c41a5
Compare
@mcollina: PTAL |
e2d3829
to
1f345b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this fix. Why is this needed at all?
(A slightly better implementation would go and replace the kOn* methods when paused to avoid the ifs on an hot path).
Because currently when pausing we don't actually pause and the parser might invoke more callbacks. It's more of a hint.
I think this is negligable. |
I guess it's not entirely necessary... though the current implementation isn't strictly correct. The docs would need to be updated to reflect that returning |
Have you benchmarked this? |
Yes, difference is within margin of error (which is quite large, we should probably increase number of samples in benchmark again). |
e2568b0
to
a0d8cb0
Compare
a0d8cb0
to
7a40258
Compare
I've cleaned this up a bit and also added more assertions to make it clearer what this fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341